v1 Rewrite #34

sheck · 2024-09-19T20:22:19Z

In this PR I rewrote the action in TypeScript and swapped Github check runs for workflow command output powered annotations. This is a culmination of a lot of internal discussion, planning, and unexpected delays.

The core motivation is this: there have been longstanding issues with Github check runs initiated from Github actions, specifically that they get associated with the incorrect action run. Github now recommends using workflow output command annotations for situations like this.

There are also other issues we wanted to address:

Check run permission headaches
Simplifying dependency install
Lack of tests
Input and output variable naming using camelCase instead of kebab-case

Breaking changes

Similar to when we changed balto-rubocop to use workflow command output, there is no way to have balto-eslint display a neutral conclusion.
- This has been heavily requested for many years now, but we're still waiting on Github
- Users can specify a conclusion-level of success (the default) or failure to control how this affects the status of Github checks in the UI.
Inputs and outputs use kebab-case now, which matches what Github (eventually) seemed to coalesce on as the standard
We no longer manage installing dependencies and instead rely on balto-utils for a cached, speedy install of all JS dependencies
- There was a non-insignificant amount of complexity involved in maintaining this feature that was implemented before caching was even possible with Github actions
The source label name for annotations is now derived from job name, not hardcoded by us (it is recommended to call this action in a job named balto-eslint for labeling to be correct)

Non-breaking changes

We have official support for ESLint v9 now!
We're using devbox to ensure we have a consistent development environment
Using act for some somewhat manual, somewhat automatic testing
I moved the CONTRIBUTING.md file to a section in the README.md

Notes for reviewing

There's a lot here! Sorry! Everything outside of src/ and action.yml are just supporting files though.

This should resolve #3 and #5

markpalfreeman · 2024-09-25T16:19:16Z

I don't feel prepared to review this PR, but if there comes a point where you need someone to test it, let me know! I have a Giving branch configured with ESLint v9 right now.

sheck · 2024-09-25T17:49:51Z

@markpalfreeman I'm meeting with the other Balto maintainers tomorrow morning to discuss this.

If you want you can change your GitHub workflow to something like this to test on this branch:

jobs:
  balto-eslint:
    runs-on: ubuntu-latest
    steps:
      - uses: planningcenter/balto-utils/yarn@v2
      - uses: planningcenter/balto-eslint@ns/v1

danielma

This looks great. Using the core package to make annotations in STDOUT is clearly the blessed path going forward, and at the end of the day it's a lot easier to reason about when it's just STDOUT rather than API interactions.

Honestly, I really only have one big question, which is about the order-dependent API of EslintResult. I think we can find an API that more directly acknowledges the async initialization process, and we'll be in good shape.

Also, a huge thank you for writing tests for this project. That makes the whole thing so much more viable to maintain and keep working on into the future 🍻

danielma · 2024-09-30T23:09:27Z

src/index.ts

+    // Eslint will return exit code 1 if it finds linting problems, but that is
+    // expected and we don't want to stop execution because of it.


Thank you for this comment

danielma · 2024-09-30T23:12:33Z

src/eslint_result.ts

+
+  async asyncInitialize() {
+    this.changeRanges = await generateChangeRanges(
+      this.resultObject.filePath,


Is filePath relative or absolute? My headspace for the question comes from noting distinction in how we build the array of changed files. Sometimes those are relative to the workingDir vs relative to the git root. However, based on my checking locally, git diff --name-only always outputs the path from the git root

That is a good question. After looking myself, it seems filePath from eslint is an absolute path. Git appears to not mind that we are calling diff on an absolute path.

The file paths that we give to eslint are from the working directory though, as we manipulate the path to scope it to the working directory here:

balto-eslint/src/git_utils.ts

Lines 53 to 55 in 2f523c7

.split("\n")

.filter((path) => path !== "")

.map((path) => path.replace(workingDirectory + "/", ""))

Nice. Ok, that makes sense. Absolute file paths from eslint seem good, and it makes sense that git would be happy with that. Thanks for answering!

danielma · 2024-09-30T23:14:17Z

src/git_utils.ts

+    .map((match) => {
+      const rangeStart = parseInt(match!.groups!.rangeStart, 10)
+      const rangeLength = match!.groups!.rangeLength
+        ? parseInt(match!.groups!.rangeLength) - 1


Suggested change

? parseInt(match!.groups!.rangeLength) - 1

? parseInt(match!.groups!.rangeLength, 10) - 1

danielma · 2024-09-30T23:24:16Z

src/eslint_result.ts

+    this.relevantMessages.forEach((msg) => {
+      switch (msg.severity) {
+        case 1:
+          this.relevantWarningCount += 1


The imperative nature of this class has me wondering if we could find a more imperative API generate the results we want. I wonder what it would look like if we waited to initialize the EslintResult until we'd generated the data we care about. Maybe we could have a static async function on EslintResult that transforms the ResultObject into a fully-formed EslintResult.

It might be something like

class EslintResult { static async function fromResult(result: ResultObject, compareSha: string) { const changeRanges = await generateChangeRanges(result.filePath, compareSha) const relevantMessages = result.messages.filter(m => changeRanges.some(cr => cr.doesInclude(m.line))) const { warningCount, errorCount } = calculateCounts(relevantMessages) return new EslintResult({ result, relevantMessages, warningCount, errorCount }) } static function calculateCounts(messages: ResultObject['messages']) { return messages.reduce((acc, message) => { switch (message.severity) { case 1: acc.warningCount += 1 break case 2: acc.errorCount += 1 break } return acc }, { warningCount: 0, errorCount: 0 }) } }

We can still initialize all the objects async (with Promise.all), but we no longer need to call instance methods in a certain order for them to be valid.

I like that idea, I'll take a look 👍

danielma · 2024-09-30T23:25:28Z

src/eslint_result.ts

+        endColumn: msg.endColumn,
+      }
+      switch (msg.severity) {
+        case 1:


Purely for readability, maybe let's extract an enum for severity

enum Severity { Warning = 1, Error = 2 }

Great call 👍

molawson

We talked this over on a call, and I feel great about this, especially the overall simplification and the addition of tests!

Instead of constructing the object and then calling an async function on it, let's do it all in one go. The simplifies the class defintion of EslintResult a bit.

danielma · 2024-10-02T00:02:13Z

That set of changes looks solid. I'm so excited to have this released!

With check runs, we could specify the naming ourselves. With the switch to workflow commands, it depends on how consumers of this action name the consuming action.

sheck added 4 commits September 19, 2024 12:36

v1 Rewrite

b621630

Undo simulated changes

c2eaebd

Update test target sha

1bbc5c2

Simulate changes

2f523c7

sheck requested a review from a team as a code owner September 19, 2024 20:22

sheck changed the title ~~Ns/v1~~ v1 Rewrite Sep 19, 2024

sheck linked an issue Sep 24, 2024 that may be closed by this pull request

Install Needed Prettier/Babel packages #5

Closed

danielma approved these changes Sep 30, 2024

View reviewed changes

fix: incorrect logging interpolation

f548cd3

molawson approved these changes Oct 1, 2024

View reviewed changes

sheck added 3 commits October 1, 2024 14:03

Add note about git diff flags

fd74aa4

refactor: initialize EslintResult with a single async function

d9cbca5

Instead of constructing the object and then calling an async function on it, let's do it all in one go. The simplifies the class defintion of EslintResult a bit.

refactor: use enum for Severity

2897587

sheck added 2 commits October 2, 2024 11:19

fix: add missing radix to parseInt

3e8aaee

docs: explain annotation naming

fe3afef

With check runs, we could specify the naming ourselves. With the switch to workflow commands, it depends on how consumers of this action name the consuming action.

sheck merged commit 627061e into main Oct 2, 2024
4 checks passed

sheck deleted the ns/v1 branch October 2, 2024 20:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v1 Rewrite #34

v1 Rewrite #34

sheck commented Sep 19, 2024 •

edited

Loading

markpalfreeman commented Sep 25, 2024

sheck commented Sep 25, 2024

danielma left a comment

danielma Sep 30, 2024

danielma Sep 30, 2024

sheck Oct 1, 2024

danielma Oct 1, 2024

danielma Sep 30, 2024

danielma Sep 30, 2024 •

edited

Loading

sheck Oct 1, 2024

danielma Sep 30, 2024

sheck Oct 1, 2024

molawson left a comment

danielma commented Oct 2, 2024

		// Eslint will return exit code 1 if it finds linting problems, but that is
		// expected and we don't want to stop execution because of it.

	.split("\n")
	.filter((path) => path !== "")
	.map((path) => path.replace(workingDirectory + "/", ""))

	? parseInt(match!.groups!.rangeLength) - 1
	? parseInt(match!.groups!.rangeLength, 10) - 1

v1 Rewrite #34

v1 Rewrite #34

Conversation

sheck commented Sep 19, 2024 • edited Loading

Breaking changes

Non-breaking changes

Notes for reviewing

markpalfreeman commented Sep 25, 2024

sheck commented Sep 25, 2024

danielma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielma Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

molawson left a comment

Choose a reason for hiding this comment

danielma commented Oct 2, 2024

sheck commented Sep 19, 2024 •

edited

Loading

danielma Sep 30, 2024 •

edited

Loading